Skip to content

Conversation

@tdas
Copy link
Contributor

@tdas tdas commented Aug 10, 2015

No description provided.

@tdas
Copy link
Contributor Author

tdas commented Aug 10, 2015

@zsxwing Could you take a quick look?

@SparkQA
Copy link

SparkQA commented Aug 10, 2015

Test build #40324 has finished for PR 8080 at commit b711214.

  • This patch fails Python style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Aug 10, 2015

Test build #1423 has finished for PR 8080 at commit 5781728.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Aug 11, 2015

Test build #1434 has finished for PR 8080 at commit 5781728.

  • This patch fails Python style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Aug 11, 2015

Test build #40377 has finished for PR 8080 at commit 60479da.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to throw an exception when activeJvmContextOption.get().hashCode() != activePythonContextJavaId because it will be confusing that getActive returns None but getActiveOrCreate still complains there is already an active context in Scala codes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was on the fence about what to do here. Thanks for deciding for me.

@SparkQA
Copy link

SparkQA commented Aug 11, 2015

Test build #1435 has finished for PR 8080 at commit 9c2da9c.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the previous code means the stdout is in python/unit-tests.log, which is correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you see how LOG_FILE is defined, it is already an absolute path.
https://github.com/tdas/spark/blob/SPARK-9572/python/run-tests.py#L69

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right. Good catch about the path.

@zsxwing
Copy link
Member

zsxwing commented Aug 11, 2015

retest this please

@SparkQA
Copy link

SparkQA commented Aug 11, 2015

Test build #40410 has finished for PR 8080 at commit e21488d.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@zsxwing
Copy link
Member

zsxwing commented Aug 11, 2015

retest this please

@zsxwing
Copy link
Member

zsxwing commented Aug 11, 2015

LGTM.

@zsxwing
Copy link
Member

zsxwing commented Aug 11, 2015

retest this please

@tdas
Copy link
Contributor Author

tdas commented Aug 11, 2015

retest this please.

@SparkQA
Copy link

SparkQA commented Aug 11, 2015

Test build #40418 has finished for PR 8080 at commit e21488d.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Aug 11, 2015

Test build #40430 has finished for PR 8080 at commit f4f094c.

  • This patch fails Python style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Aug 11, 2015

Test build #1439 has finished for PR 8080 at commit e21488d.

  • This patch fails Spark unit tests.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Aug 11, 2015

Test build #40431 has finished for PR 8080 at commit 741a0d0.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

false -> False

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldnt this be false based on Java's style of writing false

@SparkQA
Copy link

SparkQA commented Aug 11, 2015

Test build #1446 has started for PR 8080 at commit 741a0d0.

@SparkQA
Copy link

SparkQA commented Aug 11, 2015

Test build #1447 has started for PR 8080 at commit 741a0d0.

@SparkQA
Copy link

SparkQA commented Aug 11, 2015

Test build #40439 has finished for PR 8080 at commit 64a231d.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Aug 11, 2015

Test build #1448 has finished for PR 8080 at commit 64a231d.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@zsxwing
Copy link
Member

zsxwing commented Aug 11, 2015

LGTM

@asfgit asfgit closed this in 5b8bb1b Aug 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants